Conversation
There was a problem hiding this comment.
Pull request overview
This PR standardizes the “backup-family” task response surface across backup_mongo, backup_pg, and mysql_backups by extracting a shared response base and aligning response model naming, then regenerating OpenAPI snapshots and the frontend API client to match the updated schema.
Changes:
- Introduce a shared
BackupTaskBase(BaseTaskResponse)carryinghostnameand reuse it across the three backup apps. - Rename
mysql_backupsBackupResponse→BackupTaskResponseand update route/dep/test references accordingly. - Regenerate OpenAPI golden snapshots and the frontend OpenAPI spec + generated TypeScript client for the new component names.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/sep/apps/backup_responses.py | Adds shared BackupTaskBase used by all backup-family apps. |
| app/sep/apps/backup_mongo/models.py | Switches to importing the shared BackupTaskBase. |
| app/sep/apps/backup_pg/models.py | Switches to importing the shared BackupTaskBase. |
| app/sep/apps/mysql_backups/models.py | Switches to shared base and renames response model to BackupTaskResponse. |
| app/sep/apps/mysql_backups/deps.py | Updates response builder return type and construction to BackupTaskResponse. |
| app/sep/apps/mysql_backups/app.py | Updates app registration response_model to BackupTaskResponse. |
| tests/app/sep/apps/mysql_backups/test_models.py | Renames tests and usages to BackupTaskResponse. |
| tests/app/sep/snapshots/openapi/mysql_backups.json | Updates OpenAPI snapshot for renamed response model and paginated schema keys. |
| tests/app/sep/snapshots/openapi/backup_mongo.json | Updates OpenAPI snapshot for new paginated schema key suffix. |
| tests/app/sep/snapshots/openapi/backup_pg.json | Updates OpenAPI snapshot for new paginated schema key suffix. |
| frontend/packages/api/specs/sep.json | Regenerates frontend OpenAPI spec to match backend response model rename. |
| frontend/packages/api/src/generated/sep.ts | Regenerates TypeScript client/types to match updated OpenAPI components. |
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
yyyyyyyan
left a comment
There was a problem hiding this comment.
@peter-o-addo — clean, tightly-scoped extraction. The shared BackupTaskBase kills the hostname triplication exactly as the ticket asks, living alongside framework so that layer stays domain-neutral; backup_type stays optional on the mysql child (confirmed in the regenerated snapshot — it's nullable and not in required); and the rename is complete, with no lingering BackupResponse references left in app/, tests/, or frontend/. test_exposes_inherited_task_response_surface still genuinely exercises the repointed inheritance.
One thing worth noting on scope: the AC said the backup_mongo / backup_pg snapshots would be unchanged, but both moved (PaginatedResponse_BackupTaskResponse___1→___2 and ___2→___3). That's correct and unavoidable — renaming mysql's class to BackupTaskResponse makes it a three-way name collision, so FastAPI mangles all three with module-qualified keys and renumbers the order-dependent suffixes. Regenerating those snapshots was the right call; the AC just under-counted the blast radius.
A few optional docstring nits, posted inline as one-click suggestions — all cases of a :type:/:rtype: line restating an annotation the signature already carries (backup_responses.py:33, mysql_backups/models.py:651, deps.py:168).
On the two open Copilot threads:
backup_responses.py:26— the suggested change (import fromframework.responses) won't actually avoid theframework/__init__side effects: Python runs a package's__init__for any submodule import, so importingframework.responsesstill executes all offramework/__init__'s re-exports (I confirmed this directly). The real inaccuracy is the module docstring's claim that the layer is shared "without importingframework.__init__and unrelated SQLModel tables into scope" — that's the part worth softening, not the import line.test_models.py:117— agree; the "Tests for …" opener reads as a noun phrase, so an imperative verb (e.g. "Cover …") fits the convention.
Nothing blocking — all findings are cosmetic. Approving.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
yyyyyyyan
left a comment
There was a problem hiding this comment.
@peter-o-addo — all four points from the last round are addressed: BackupTaskBase now lives at app/sep/apps/shared/backups/responses.py alongside columns.py / edit_form.py, it is exported through shared/backups/__init__.py, the three consumers import it from the submodule (matching how BACKUP_TYPE_COLUMN and parse_server_list_config are already imported), and the three redundant :type: / :rtype: directives are gone. No new issues in the fix commits.
Minor — stale path in the PR Summary. The Summary still lists app/sep/apps/backup_responses.py : new shared BackupTaskBase(BaseTaskResponse); that module was removed by the move. Worth updating the bullet to the new path before merge so the description matches the diff.
LGTM.




Summary
Extract a shared backup-family
BackupTaskBase(hostname only) intoapp/sep/apps/backup_responses.py, repoint the three backup apps at it, and rename mysql_backups'BackupResponsetoBackupTaskResponseso all backup apps share the same response naming.app/sep/apps/backup_responses.py: new sharedBackupTaskBase(BaseTaskResponse)carryinghostname: str | None = Noneapp/sep/apps/backup_mongo/models.py,app/sep/apps/backup_pg/models.py: removed localBackupTaskBase; import shared base;BackupTaskResponse/ detail models unchangedapp/sep/apps/mysql_backups/models.py: import shared base; renamedBackupResponse→BackupTaskResponse; movedbackup_type: BackupType | Noneonto the child (hostname inherited from shared base)app/sep/apps/mysql_backups/app.py,app/sep/apps/mysql_backups/deps.py: updated imports,response_model, builder return type, andbuild_default_task_responsecall toBackupTaskResponsetests/app/sep/snapshots/openapi/mysql_backups.json,tests/app/sep/snapshots/openapi/backup_mongo.json,tests/app/sep/snapshots/openapi/backup_pg.json: regenerated OpenAPI goldens forBackupResponse→BackupTaskResponserename and updatedPaginatedResponse[BackupTaskResponse]disambiguation suffixesfrontend/packages/api/specs/sep.json,frontend/packages/api/src/generated/sep.ts: regenerated frontend spec and TS client to match the mysql_backups rename and pagination schema key changestests/app/sep/apps/mysql_backups/test_models.py: renamedBackupResponsereferences and test class toBackupTaskResponseTested
Checklist
make test)make run-pre-commit)make makemigrations)changelog.d/if the change is user-facing (make changelog-add), or confirmed N/A (internal-only change, or a same-release-cycle fix for an unreleased sibling ticket)